Engine server hook #347

bruno- · 2024-04-18T12:59:13Z

Hi,

this is a proof of concept for automatically running watch mode alongside any rails server in development.

This is the alternative solution to work done in #300. While puma plugin from that PR works only with puma, this approach works with any server (puma included, of course).

I'm looking for some feedback from the maintainers, also from @npezza93 , @javierav because it affects #327 and #331.

lib/tailwindcss/engine.rb

which would break the debugger.

flavorjones · 2024-04-27T16:42:05Z

I've rebased this branch and added a commit to avoid breaking the debugger (see #346 and #349 for context).

flavorjones

I really like this approach, however the final version needs a few things:

needs to only run in development mode, as @bruno- and @brunoprietog discussed in their thread
needs to only run in new installations

To explain (2) above, this is the scenario we must avoid:

user installs an older tailwindcss-rails version
user configures either Procfile.dev and foreman or the Puma plugin to manage the watch process
user upgrades to a version of tailwindcss-rails with this watcher
when the user runs their dev server they now have two tailwindcss watchers

One idea is to have the generator create an initializer that sets something like:

    config.tailwindcss_rails_server = true

and then engine.rb can be patched like:

diff --git a/lib/tailwindcss/engine.rb b/lib/tailwindcss/engine.rb
index ead1c9b9..96c87621 100644
--- a/lib/tailwindcss/engine.rb
+++ b/lib/tailwindcss/engine.rb
@@ -15,6 +15,7 @@ class Engine < ::Rails::Engine
     end
 
     server do
+      if Rails.application.config.respond_to?(:tailwindcss_rails_server) && Rails.application.config.tailwindcss_rails_server
         tailwind_pid = fork do
           # To avoid stealing keystrokes from the debug gem's IRB console in the main process (which
           # needs to be able to read from $stdin), we use `IO.open(..., 'r+')`.
@@ -28,3 +29,4 @@ class Engine < ::Rails::Engine
       end
     end
   end
+end

Does that make sense? I'm open to other ideas.

brunoprietog · 2024-04-27T19:34:24Z

How about a breaking change for a 3.0 version with an update notice? If this is merged, hopefully, we would have 3 ways to run the gem, which means more unnecessary confusion for devs. Couldn't this replace all the paths? Why would we need the other methods having this?

flavorjones · 2024-04-29T02:19:42Z

@brunoprietog I agree we don't need to support multiple ways of running the watcher in a server if this gets merged, so I intend to remove the other methods, eventually.

However, I'd prefer to separate the introduction of the server hook from the removal of the other methods of running the watcher. Having a deprecation period, even a short one, will allow people to choose when to upgrade. The cost of doing so is not very high, IMHO, and it will save us from having to deal with the support burden.

flavorjones · 2024-04-29T02:21:31Z

@bruno- Let me know if you want to try to finish this up per my comments above? If not, I'll find time to work on it this week -- I just don't want to duplicate efforts! Thanks.

bruno- · 2024-04-29T10:54:11Z

@flavorjones thank you for green-lighting this feature.

I can make changes based on your suggestions.

bruno- · 2024-04-30T06:45:07Z

Quick update - I addressed everything we talked in this thread, but I encountered a problem:

Puma can be restarted via touch tmp/restart.txt or by sending a USR1 signal to the process.
It then calls Kernel.exec, reinitializes everything and consequently calls the Engine.server block again - which leads to multiple tailwindcss processes.
Since Kernel.exec is used, there's no way to maintain a flag like tailwindcss_process_started = true anywhere in the server process. Tailwindcss process doesn't detect changes in the server process.

Some of the approaches I'm thinking about:

Use tmp/tailwindcss_pid.txt
Leave this as-is, document this as a limitation/known bug.
Killing existing tailwindcss subprocess(es) when running server block.

bruno- · 2024-04-30T08:07:25Z

lib/tailwindcss/engine.rb

@@ -2,6 +2,9 @@

 module Tailwindcss
  class Engine < ::Rails::Engine
+    config.tailwindcss = ActiveSupport::OrderedOptions.new
+    config.tailwindcss.server_process = false # Rails.env.development?


If (or when) you decide to make "server process" the default for all:

Just set the tailwindcss.server_process option value to Rails.env.development? (propshaft does it like that)

Optionally remove the if DEVELOPMENT_ENVIRONMENT_CONFIG_PATH.exist? section from lib/install/tailwindcss.rb

bruno- · 2024-04-30T08:10:04Z

lib/install/tailwindcss.rb

@@ -16,6 +17,15 @@
  say %(        Add <%= stylesheet_link_tag "tailwind", "inter-font", "data-turbo-track": "reload" %> within the <head> tag in your custom layout.)
 end

+if DEVELOPMENT_ENVIRONMENT_CONFIG_PATH.exist?


This will enable the "server process" only for new installs.

I opted for this solution over creating an initializer file because:

It's one file less to manage

Config options belong to config/environments/*, not initializers

bruno- · 2024-04-30T08:13:15Z

lib/tailwindcss/server_process.rb

+      end
+    end
+
+    module Pidfile


Pidfile is a solution to a problem described here: #347 (comment)

bruno- · 2024-04-30T08:16:58Z

@flavorjones this PR is ready for another look.

bruno- · 2024-04-30T09:00:37Z

lib/tailwindcss/server_process.rb

+
+    module Pidfile
+      def self.path
+        Rails.root.join("tmp", "pids", "tailwindcss.txt")


New rails app already have tmp/pids/ dir https://github.com/rails/rails/blob/92591ebd54e044ff3f3b9d802fe65f69cb7e34f2/railties/lib/rails/generators/rails/app/app_generator.rb#L257-L260

This is similar to turning the cache on or off, touch restart.txt, etc.

flavorjones · 2024-05-01T14:04:03Z

@bruno- Thanks for continuing your work on this, in particular the care you took around handling Puma restarts. I tried the code out on my local machine and it all seems to work for me.

It's a sizable chunk of new code, though, which is making me hesitate to ship it in this gem, especially since other gems like cssbundling-rails and dartsass-rails might also be able to use this approach to avoid the Procfile.dev/foreman hoop-jumping.

I'm also curious about test coverage; by any chance was this code adapted from an existing project that tests the edge cases (and if so, should we make sure to pay attention to any licensing requirements)?

I would consider putting the process-management code into a new gem, and taking a dependency on that gem, if we can both add some test coverage and use it in other gems. Or: is there an existing gem that does this already? (I searched briefly but wasn't satisfied with the results.)

@rafaelfranca we had a chat a few months back about providing some shared code for the asset builders. Any thoughts here?

bruno- · 2024-05-03T07:57:59Z

Hi @flavorjones

It's a sizable chunk of new code, though, which is making me hesitate to ship it in this gem

I get it. Let's take a step back and consider our options.

I'm also curious about test coverage; by any chance was this code adapted from an existing project

No, this code was not adapted from any existing project. I started looking at the code in lib/puma/plugin/tailwindcss.rb and took it from there. My only other references were docs.ruby-lang.org and stackoverflow 😅

...from an existing project that tests the edge cases

I, myself spent quite a bit of time manually testing this, discovering bugs etc.

should we make sure to pay attention to any licensing requirements?

I described my working process above. I confirm I'm the author of the code, we can license it however we want.

I would consider putting the process-management code into a new gem, and taking a dependency on that gem, if we can both add some test coverage and use it in other gems.

I think extracting this into a new gem is a good idea 👍
Yes, I can add tests.
Yes, the gem could be used for other rails gems that need "watcher" processes.

Is there an existing gem that does this already?

I'm also not aware of any other gem.

bruno- · 2024-05-03T08:28:25Z

I'll extract this code to a gem and add tests.

The only question I have is: do you think "server process" is a good name for the gem, and this feature in general?

dhh · 2024-07-29T15:14:41Z

I'm not loving the heavy and TW specific ServerProcess implementation here. I think there's definitely something to the idea, but it can't be some TW bespoke thing. It needs to be a generic Rails feature that's then just used here.

flavorjones · 2024-07-29T17:36:52Z

Agree. @brunoprietog let me know if you'd like to collaborate on something we could use for Rails generally, happy to make time.

brunoprietog · 2024-07-29T17:40:12Z

Happy to collaborate here! But the author of this is @bruno-. It's a fun coincidence that we are both named Bruno 😂

flavorjones · 2024-07-30T18:30:15Z

Sorry about that! Autocomplete fail. 🤣

coorasse · 2024-09-11T08:08:27Z

Has anyone already explored the option to have it integrated in the same rails process as a middleware? Basically what sprockets does? Sure, it would be slower, but you can always just run it in a separate thread a-la webpacker

bruno- commented Apr 18, 2024

View reviewed changes

lib/tailwindcss/engine.rb Show resolved Hide resolved

bruno- commented Apr 18, 2024

View reviewed changes

lib/tailwindcss/engine.rb Outdated Show resolved Hide resolved

bruno- marked this pull request as draft April 20, 2024 20:22

bruno- and others added 2 commits April 27, 2024 12:38

Engine server hook

f77a0ce

The tailwindcss watch process must not steal keystrokes

534a840

which would break the debugger.

flavorjones force-pushed the engine-server branch from 751a511 to 534a840 Compare April 27, 2024 16:41

flavorjones requested changes Apr 27, 2024

View reviewed changes

flavorjones marked this pull request as ready for review April 27, 2024 17:02

flavorjones mentioned this pull request Apr 27, 2024

Install as Puma plugin if available #331

Draft

bruno- added 3 commits April 29, 2024 21:57

Use ServerProcess class

355ed62

Add config.tailwindcss.server_process option

eed5343

Enable server process only for new installs

7265220

bruno- force-pushed the engine-server branch from 6dd82b6 to 7265220 Compare April 29, 2024 21:48

bruno- commented Apr 30, 2024

View reviewed changes

Ensure ServerProcess is started only once

b4879aa

bruno- force-pushed the engine-server branch from 5b161b3 to b4879aa Compare April 30, 2024 08:11

bruno- commented Apr 30, 2024

View reviewed changes

lib/tailwindcss/server_process.rb

end

end

module Pidfile

Copy link

Author

bruno- Apr 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pidfile is a solution to a problem described here: #347 (comment)

bruno- requested a review from flavorjones April 30, 2024 08:17

bruno- commented Apr 30, 2024

View reviewed changes

brunoprietog mentioned this pull request Jul 29, 2024

Add bin/dev by default rails/rails#52433

Merged

flavorjones closed this Jul 29, 2024

dhh mentioned this pull request Jul 30, 2024

Run the watcher on a Thread when Rails boots instead of foreman ? rails/dartsass-rails#58

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Engine server hook #347

Engine server hook #347

bruno- commented Apr 18, 2024

flavorjones commented Apr 27, 2024

flavorjones left a comment

brunoprietog commented Apr 27, 2024

flavorjones commented Apr 29, 2024

flavorjones commented Apr 29, 2024

bruno- commented Apr 29, 2024

bruno- commented Apr 30, 2024

bruno- Apr 30, 2024

bruno- Apr 30, 2024

bruno- Apr 30, 2024

bruno- commented Apr 30, 2024

bruno- Apr 30, 2024

simi Jul 29, 2024

brunoprietog Jul 29, 2024

flavorjones commented May 1, 2024

bruno- commented May 3, 2024

bruno- commented May 3, 2024

dhh commented Jul 29, 2024

flavorjones commented Jul 29, 2024

brunoprietog commented Jul 29, 2024

flavorjones commented Jul 30, 2024

coorasse commented Sep 11, 2024

Engine server hook #347

Engine server hook #347

Conversation

bruno- commented Apr 18, 2024

flavorjones commented Apr 27, 2024

flavorjones left a comment

Choose a reason for hiding this comment

brunoprietog commented Apr 27, 2024

flavorjones commented Apr 29, 2024

flavorjones commented Apr 29, 2024

bruno- commented Apr 29, 2024

bruno- commented Apr 30, 2024

bruno- Apr 30, 2024

Choose a reason for hiding this comment

bruno- Apr 30, 2024

Choose a reason for hiding this comment

bruno- Apr 30, 2024

Choose a reason for hiding this comment

bruno- commented Apr 30, 2024

bruno- Apr 30, 2024

Choose a reason for hiding this comment

simi Jul 29, 2024

Choose a reason for hiding this comment

brunoprietog Jul 29, 2024

Choose a reason for hiding this comment

flavorjones commented May 1, 2024

bruno- commented May 3, 2024

bruno- commented May 3, 2024

dhh commented Jul 29, 2024

flavorjones commented Jul 29, 2024

brunoprietog commented Jul 29, 2024

flavorjones commented Jul 30, 2024

coorasse commented Sep 11, 2024